Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add index error handling #11

Closed
wants to merge 4 commits into from
Closed

Conversation

stevebauman
Copy link
Contributor

Purpose

This PR adds an exception to be thrown when an index response contains errors.

Description

I discovered after a couple hours of troubleshooting why my indexing wasn't working with my custom mapping using elastic-migrations. I had to jump into my vendor directory and dump the response ES was replying back with, but unfortunately no errors are handled, logged, or returned.

Without any error handling, I can't use this package in production, as my indexing may suddenly stop working and I won't have any knowledge of its failure without enabling some sort of logging mechanism inside of ElasticSearch itself.

References

This PR abides by ElasticSearch's outlined response body for bulk operations.

https://www.elastic.co/guide/en/elasticsearch/reference/master/docs-bulk.html#bulk-api-response-body


Let me know if you would like anything adjusted or have any comments/criticisms of this PR. I'm all for abiding by whatever style you would like 👍

Thanks!

@stevebauman
Copy link
Contributor Author

We may want to include the full response inside the ElasticException so it can be retrieved by the developer (would like your thoughts on this):

 throw (new ElasticException(
    sprintf('%s: %s', $error['type'], $error['reason'])
))->setResponse($response);
try {
    // ...
} catch (ElasticException $e) {
    $e->getResponse();
}

@babenkoivan
Copy link
Owner

Hey, @stevebauman thank you for the PR!

This is a known issue, I was hoping that it gets resolved on elasticsearch client side, but as more people face the issue, it makes sense to resolve it in the adapter.

I was working on a similar approach, but never pushed it until now (see v1.16.1). You can update the composer and start catching custom BulkRequestException:

try {
    // ...
} catch (BulkRequestException $e) {
    $response = $e->getResponse();
}

@babenkoivan babenkoivan closed this Jun 3, 2021
@stevebauman
Copy link
Contributor Author

Ah perfect @babenkoivan! Thanks for pushing an update! Really appreciate it 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants